- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 33.6k
http: Make OutgoingMessage more streamlike #45672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
          
     Merged
      
      
    
                
     Merged
            
            
          Conversation
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
    | Review requested: 
 | 
0af46f6    to
    ca3ed8a      
    Compare
  
    Implement missing getters error & closed. Add support for proper "writable" check through isWritable helper. We cannot fix the OutgoingMessage.writable propery as that will break the ecosystem.
ca3ed8a    to
    1047db4      
    Compare
  
    
              
                    aduh95
  
              
              reviewed
              
                  
                    Nov 29, 2022 
                  
              
              
            
            
| The changes LGTM. Are you going to add any test? | 
      
        
      
      
  
    15 tasks
  
              
                    mcollina
  
              
              approved these changes
              
                  
                    Nov 30, 2022 
                  
              
              
            
            
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
              
                    aduh95
  
              
              reviewed
              
                  
                    Nov 30, 2022 
                  
              
              
            
            
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are seemingly related test failures.
Co-authored-by: Antoine du Hamel <[email protected]>
      
        
      
      
  
    10 tasks
  
              
                    Flarna
  
              
              approved these changes
              
                  
                    Dec 1, 2022 
                  
              
              
            
            
      
        
      
      
  
    10 tasks
  
              
                    JungMinu
  
              
              approved these changes
              
                  
                    Dec 2, 2022 
                  
              
              
            
            
              
                    aduh95
  
              
              approved these changes
              
                  
                    Dec 2, 2022 
                  
              
              
            
            
| Landed in d385106 | 
  This was referenced Dec 3, 2022 
      
    
  targos 
      pushed a commit
      that referenced
      this pull request
    
      Dec 12, 2022 
    
    
      
  
    
      
    
  
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
    
  danielleadams 
      pushed a commit
      that referenced
      this pull request
    
      Dec 30, 2022 
    
    
      
  
    
      
    
  
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
    
  danielleadams 
      pushed a commit
      that referenced
      this pull request
    
      Dec 30, 2022 
    
    
      
  
    
      
    
  
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
    
  danielleadams 
      pushed a commit
      that referenced
      this pull request
    
      Jan 3, 2023 
    
    
      
  
    
      
    
  
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
    
  danielleadams 
      pushed a commit
      that referenced
      this pull request
    
      Jan 4, 2023 
    
    
      
  
    
      
    
  
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
    
  danielleadams 
      pushed a commit
      that referenced
      this pull request
    
      Jan 5, 2023 
    
    
      
  
    
      
    
  
Implement missing getters error & closed. Add support for proper "writable" check through `isWritable` helper. We cannot fix the `OutgoingMessage.writable` property as that would break the ecosystem. PR-URL: #45672 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
    
  ijjk 
      pushed a commit
        to vercel/next.js
      that referenced
      this pull request
    
      Jul 26, 2023 
    
    
      
  
    
      
    
  
### What?
This reimplements our stream cancellation code for a few more cases:
1. Adds support in all stream-returning APIs
2. Fixes cancellation detection in node 16
3. Implements out-of-band detection, so can cancel in the middle of a
read
It also (finally) adds tests for all the cases I'm aware of.
### Why?
To allow disconnecting from an AI service when a client disconnects. $$$
### How?
1. Reuses a single pipe function in all paths to push data from the
dev's `ReadableStream` into our `ServerResponse`
2. Uses `ServerResponse` to detect disconnect, instead of the
`IncomingMessage` (request)
    - The `close` event fire once all incoming body data is read
- The request `abort` event will not fire after the incoming body data
has been fully read
3. Using `on('close')` on the writable destination allows us to detect
close
- Checking for `res.destroyed` in the body of the loop meant we had to
wait for the `await stream.read()` to complete before we could possibly
cancel the stream
- - -
#52157 (and #51594) had an issue with Node 16, because I was using
`res.closed` to detect when the server response was closed by the client
disconnecting. But, `closed` wasn't
[added](nodejs/node#45672) until
[v18.13.0](https://nodejs.org/en/blog/release/v18.13.0#:~:text=%5Bcbd710bbf4%5D%20%2D%20http%3A%20make%20OutgoingMessage%20more%20streamlike%20(Robert%20Nagy)%20%2345672).
This fixes it by using `res.destroyed`.
Reverts #52277
Relands #52157
Fixes #52809
---------
    
    
  Strift 
      pushed a commit
        to Strift/next.js
      that referenced
      this pull request
    
      Jul 27, 2023 
    
    
      
  
    
      
    
  
### What?
This reimplements our stream cancellation code for a few more cases:
1. Adds support in all stream-returning APIs
2. Fixes cancellation detection in node 16
3. Implements out-of-band detection, so can cancel in the middle of a
read
It also (finally) adds tests for all the cases I'm aware of.
### Why?
To allow disconnecting from an AI service when a client disconnects. $$$
### How?
1. Reuses a single pipe function in all paths to push data from the
dev's `ReadableStream` into our `ServerResponse`
2. Uses `ServerResponse` to detect disconnect, instead of the
`IncomingMessage` (request)
    - The `close` event fire once all incoming body data is read
- The request `abort` event will not fire after the incoming body data
has been fully read
3. Using `on('close')` on the writable destination allows us to detect
close
- Checking for `res.destroyed` in the body of the loop meant we had to
wait for the `await stream.read()` to complete before we could possibly
cancel the stream
- - -
vercel#52157 (and vercel#51594) had an issue with Node 16, because I was using
`res.closed` to detect when the server response was closed by the client
disconnecting. But, `closed` wasn't
[added](nodejs/node#45672) until
[v18.13.0](https://nodejs.org/en/blog/release/v18.13.0#:~:text=%5Bcbd710bbf4%5D%20%2D%20http%3A%20make%20OutgoingMessage%20more%20streamlike%20(Robert%20Nagy)%20%2345672).
This fixes it by using `res.destroyed`.
Reverts vercel#52277
Relands vercel#52157
Fixes vercel#52809
---------
    
  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment
  
      Labels
      
    author ready
  PRs that have at least one approval, no pending requests for changes, and a CI started. 
  
    http
  Issues or PRs related to the http subsystem. 
  Add this suggestion to a batch that can be applied as a single commit.
  This suggestion is invalid because no changes were made to the code.
  Suggestions cannot be applied while the pull request is closed.
  Suggestions cannot be applied while viewing a subset of changes.
  Only one suggestion per line can be applied in a batch.
  Add this suggestion to a batch that can be applied as a single commit.
  Applying suggestions on deleted lines is not supported.
  You must change the existing code in this line in order to create a valid suggestion.
  Outdated suggestions cannot be applied.
  This suggestion has been applied or marked resolved.
  Suggestions cannot be applied from pending reviews.
  Suggestions cannot be applied on multi-line comments.
  Suggestions cannot be applied while the pull request is queued to merge.
  Suggestion cannot be applied right now. Please check back later.
  
    
  
    
Implement missing getters error & closed. Add support for proper "writable" check through isWritable helper.
We cannot fix the OutgoingMessage.writable propery as that will break the ecosystem.